Add addr pool in rule #550

NikitaUnisikhin · 2023-12-11T06:31:54Z

-- Added address to the rule. Address supported IPv4/IPv6 with mask and hostname
-- Сhanged the logs taking into account the new parameter
-- Сreated a module for working with addresses (address.c) also used the module in hba
-- Added tests for new functionality

AndrewOvvv · 2024-01-24T06:48:01Z

sources/address.c

+int od_address_range_read_prefix(od_address_range_t *address_range, char *prefix)
+{
+	char *end = NULL;
+	long len = strtoul(prefix, &end, 10);


Why do we need to write an unsigned long value to a long variable? I think we should either change the type of the variable to unsigned long or use the strtol() function to convert the value to long.

len can be <= 0 (str 55)

but why do you use strtoul (to unsigned long)?

oh yeah, thanks for the comment, I'll fix it

AndrewOvvv · 2024-01-24T07:17:21Z

sources/address.c

+		if (len > 32)
+			return -1;
+		struct sockaddr_in *addr = (struct sockaddr_in *)&address_range->mask;
+		long mask;


Why do we need to store the mask as a long variable, when it can be perceived as a 32-bit integer? It might be more convenient to save it as an int32 or a uint32 type, since we do so on line 44 and in the call to the function on line 47. In both cases, it would seem more suitable to use a uint32 variable to me.

I think that it is not good idea to used signed type for mask. There're exist some UB moments with signed integer types and bitwise operations

thanks, fixed

AndrewOvvv · 2024-01-24T07:34:28Z

sources/address.c

+				return (int) ch1 - (int) ch2;
+		}
+		if (ch1 == 0)
+			break;


Why do we believe that s1 is shorter than s2? Why are we only examining ch1 and not ch2?

It's expected that ch1==ch2 at this point...
but, idk, there's strcasecmp() https://pubs.opengroup.org/onlinepubs/9699919799/functions/strcasecmp.html

this case is considered in the next iteration of the loop

AndrewOvvv · 2024-01-24T07:45:00Z

sources/address.c

+	found = false;
+	for (gai = gai_result; gai; gai = gai->ai_next)
+	{
+		if (gai->ai_addr->sa_family == client_sa->ss_family)


I'm not sure, but doesn't the "od_address_equals" function do all the things that happen inside this "if"?

thanks, fixed

x4m

Generally looks good to me, however feels very leaky. Let's make sure all allocations are free()ed correctly.

x4m · 2024-01-29T06:04:34Z

sources/address.c

+bool od_address_range_equals(od_address_range_t *first, od_address_range_t *second)
+{
+	if (first->is_hostname == second->is_hostname)
+		return first->string_value == second->string_value;


od_address_strcasecmp()?

thanks, fixed

x4m · 2024-01-29T06:08:35Z

sources/address.c

+				return (int) ch1 - (int) ch2;
+		}
+		if (ch1 == 0)
+			break;


It's expected that ch1==ch2 at this point...
but, idk, there's strcasecmp() https://pubs.opengroup.org/onlinepubs/9699919799/functions/strcasecmp.html

x4m · 2024-01-29T06:10:40Z

sources/address.c

+		return 1;
+}
+
+uint32 od_address_bswap32(uint32 x)


BTW we have od_hba_bswap32().
But I'd prefer to have od_bswap32() once and for all :)

thanks, done

x4m · 2024-01-29T06:15:11Z

sources/router.c

@@ -348,14 +349,24 @@ od_router_status_t od_router_route(od_router_t *router, od_client_t *client)

 	/* match latest version of route rule */
 	od_rule_t *rule;
+
+	struct sockaddr_storage sa;
+	if (!client->is_watchdog) {


Why do we exempt watchdog? Can we exampt all outgoing clients, not just watchdog?

thanks, now we exempt all internal clients

x4m · 2024-01-29T06:15:55Z

sources/address.c

+	return address_range;
+}
+
+int od_address_range_copy(od_address_range_t *src, od_address_range_t *dst)


maybe free() hostname?

I don't use malloc

x4m · 2024-01-29T06:17:02Z

sources/rules.c

@@ -178,6 +178,8 @@ void od_rules_rule_free(od_rule_t *rule)
 		free(rule->db_name);
 	if (rule->user_name)
 		free(rule->user_name);
+	if (rule->address_range.string_value)


maybe free address_range too?

I don't use malloc

AndrewOvvv · 2024-01-31T10:16:43Z

sources/address.c

-		long mask;
+		struct sockaddr_in *addr =
+			(struct sockaddr_in *)&address_range->mask;
+		uint32 mask;
 		if (len > 0)
 			mask = (0xffffffffUL << (32 - (int)len)) & 0xffffffffUL;


it seems that you now don't need "& 0xffffffffUL" here

thanks, fixed

NikitaUnisikhin added 30 commits December 1, 2023 21:35

add address reading

773e068

add new config for debug

1a1b957

chenge config for makefile

6911827

fixes

d388582

fix

c01311d

fix

a704207

fix

367a0d0

change config

5718198

fix

940be0a

fix

0354740

fix

da5e235

fix

841ce12

fix

6b79ba2

fix

703b076

add addr in route forward

0504b21

fix

2e95cab

fix

46f2a25

change conf

2e33c6b

fix conf

05400c9

start refactoring, add address.c

f585306

fix

95db528

fix

47b4854

fix

a02472f

fix

1384a36

fix

bfb8f11

fix

db6c190

fix

50526f2

fix

5952663

fix

9cc70a0

fix

ff135bd

NikitaUnisikhin added 17 commits December 19, 2023 14:19

add od_address_strcasecmp and od_address_hostname_match

e1432af

add od_address_check_hostname

7a0d00d

fix

33219cd

fix

a4442e2

fix

808dfca

refactoring

1058535

rm cast sockaddr to sockaddr_storage

bba1323

fixes

8fc9e5c

add od_address_check_hostname in od_address_validate

2ad0319

refactoring

48701ea

fix

d99bcba

fix

58b11f7

fix

fd2ee51

refactoring

e5b0661

fixes

93efefa

fix hostname regular

6c70a29

add tests

d071192

NikitaUnisikhin force-pushed the add_addr_pool_in_rule branch from a73e7a5 to d071192 Compare December 20, 2023 13:31

fix rule-address tests

bca8f46

AndrewOvvv reviewed Jan 24, 2024

View reviewed changes

x4m requested changes Jan 29, 2024

View reviewed changes

AndrewOvvv reviewed Jan 31, 2024

View reviewed changes

NikitaUnisikhin force-pushed the add_addr_pool_in_rule branch from 5704171 to bca8f46 Compare January 31, 2024 10:21

NikitaUnisikhin and others added 5 commits January 31, 2024 14:11

fixes

16ed94c

apply fmt

9c19743

fix od_address_check_hostname

8161ef2

apply fmt

3b45fd3

Merge branch 'master' into add_addr_pool_in_rule

40ed6f5

x4m approved these changes Feb 14, 2024

View reviewed changes

x4m merged commit f60df91 into yandex:master Feb 14, 2024
1 check failed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add addr pool in rule #550

Add addr pool in rule #550

NikitaUnisikhin commented Dec 11, 2023 •

edited

Loading

AndrewOvvv Jan 24, 2024 •

edited

Loading

NikitaUnisikhin Jan 29, 2024 •

edited

Loading

AndrewOvvv Jan 29, 2024

NikitaUnisikhin Jan 29, 2024

AndrewOvvv Jan 24, 2024 •

edited

Loading

AndrewOvvv Jan 25, 2024

NikitaUnisikhin Jan 31, 2024

AndrewOvvv Jan 24, 2024 •

edited

Loading

x4m Jan 29, 2024

NikitaUnisikhin Jan 29, 2024

AndrewOvvv Jan 24, 2024 •

edited

Loading

NikitaUnisikhin Jan 31, 2024

x4m left a comment

x4m Jan 29, 2024

NikitaUnisikhin Jan 31, 2024

x4m Jan 29, 2024

x4m Jan 29, 2024

NikitaUnisikhin Jan 31, 2024

x4m Jan 29, 2024

NikitaUnisikhin Jan 31, 2024

x4m Jan 29, 2024

NikitaUnisikhin Feb 1, 2024

x4m Jan 29, 2024

NikitaUnisikhin Feb 1, 2024

AndrewOvvv Jan 31, 2024 •

edited

Loading

NikitaUnisikhin Jan 31, 2024

Add addr pool in rule #550

Add addr pool in rule #550

Conversation

NikitaUnisikhin commented Dec 11, 2023 • edited Loading

AndrewOvvv Jan 24, 2024 • edited Loading

Choose a reason for hiding this comment

NikitaUnisikhin Jan 29, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AndrewOvvv Jan 24, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AndrewOvvv Jan 24, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AndrewOvvv Jan 24, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

x4m left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AndrewOvvv Jan 31, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NikitaUnisikhin commented Dec 11, 2023 •

edited

Loading

AndrewOvvv Jan 24, 2024 •

edited

Loading

NikitaUnisikhin Jan 29, 2024 •

edited

Loading

AndrewOvvv Jan 24, 2024 •

edited

Loading

AndrewOvvv Jan 24, 2024 •

edited

Loading

AndrewOvvv Jan 24, 2024 •

edited

Loading

AndrewOvvv Jan 31, 2024 •

edited

Loading